Support env-backed choices with choices env=...#548
Support env-backed choices with choices env=...#548
choices env=...#548Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the application's argument parsing capabilities by allowing choices for arguments and flags to be dynamically sourced from environment variables. This provides greater flexibility in configuring the application's behavior based on the environment in which it is run. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully introduces support for environment-backed choices using the choices env=... syntax. The implementation is thorough, covering parsing, shell completion, documentation generation, and KDL serialization. The refactoring of choice validation into a dedicated function improves code organization and readability. New tests adequately cover the added functionality, ensuring correctness and robustness.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a84853de12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lib/src/spec/choices.rs
Outdated
| let env_value = env | ||
| .and_then(|env_map| env_map.get(env_key).cloned()) | ||
| .or_else(|| std::env::var(env_key).ok()); |
There was a problem hiding this comment.
Parser::with_env isolated from process env
When callers use Parser::with_env to parse against a supplied environment map, this path still falls back to std::env::var, so choices env=... can be validated against ambient host variables that were never provided by the caller. That breaks the documented with_env contract in lib/src/parse.rs (“instead of the process environment”) and makes parsing nondeterministic in tests/monorepo tooling—for example, with_env(HashMap::new()) will still reject inputs if the parent process happens to export the same choice variable.
Useful? React with 👍 / 👎.
cli/src/cli/generate/fig.rs
Outdated
| template: FigArg::get_template(&arg.name), | ||
| generators: FigArg::get_generator(&arg.name), | ||
| suggestions: arg.choices.clone().map(|c| c.choices).unwrap_or_default(), | ||
| suggestions: arg.choices.as_ref().map(|c| c.values()).unwrap_or_default(), |
There was a problem hiding this comment.
usage generate fig writes a static completion file, but this now resolves choices env=... immediately into suggestions. That means the generated script snapshots whatever environment was present when usage generate fig ran (or nothing at all if the variable was unset), so users of a checked-in/CI-generated Fig spec will see stale or empty suggestions instead of their own runtime env-backed choices.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Parser::parse] --> B{custom env provided?}
B -- yes --> C[values_with_env with custom map]
B -- no --> D[values_with_env falls back to process env]
C --> E{env key in map?}
E -- yes --> F[split on commas/whitespace, deduplicate]
E -- no --> G[return only literal choices]
D --> H{env var in process env?}
H -- yes --> F
H -- no --> G
F --> I[combined literal + env values]
G --> I
I --> J{value in combined list?}
J -- no --> K{is_help_arg?}
K -- yes --> L[push help error, return Ok true]
K -- no --> M{choices.env set AND values empty?}
M -- yes --> N[bail: no choices resolved from env X]
M -- no --> O[bail: expected one of ...]
J -- yes --> P[Ok false - validation passed]
Q[Shell Completion complete_arg] --> R[choices.values - uses process env]
R --> S{env var set?}
S -- yes --> T[return env-backed completions]
S -- no --> U[return empty list]
U --> V{has_explicit_choices?}
V -- yes --> W[no file fallback]
V -- no --> X[file completion fallback]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Addressed review feedback in Changes made:
I could not run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55196528b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lib/src/parse.rs
Outdated
| let values = choices.values_with_env(custom_env); | ||
| if !values.is_empty() && !values.iter().any(|choice| choice == value) { |
There was a problem hiding this comment.
For specs that use only choices env=..., values_with_env returns an empty list when that variable is unset, and this guard then skips validation entirely. That means any explicit CLI value is accepted even though the spec declared a constrained choice set, so a missing env var in CI or a custom Parser::with_env map silently disables the new restriction instead of failing fast.
Useful? React with 👍 / 👎.
lib/src/spec/builder.rs
Outdated
| self.inner.choices = Some(SpecChoices { | ||
| choices: choices.into_iter().map(Into::into).collect(), | ||
| env: None, | ||
| }); |
There was a problem hiding this comment.
choices_env when choices() is called later
The new builder API is order-sensitive here: choices() recreates SpecChoices with env: None, so .choices_env("DEPLOY_ENVS").choices(["local"]) drops the env-backed values completely. Callers building specs programmatically can easily hit this and end up with parser/completion behavior that no longer matches the intended combined literal+env choices.
Useful? React with 👍 / 👎.
|
Addressed the two additional review comments in Updates:
I also updated the PR description to the v3 message. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c49eff4d7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cli/src/cli/complete_word.rs
Outdated
| if let Some(choices) = &arg.choices { | ||
| return Ok(choices | ||
| .choices | ||
| .iter() | ||
| .map(|c| (c.clone(), String::new())) | ||
| .filter(|(c, _)| c.starts_with(ctoken)) | ||
| .collect()); | ||
| let values = choices.values(); | ||
| if !values.is_empty() { |
There was a problem hiding this comment.
When an arg/flag is constrained only by choices env=... and that variable is unset in the completion process, choices.values() is empty and this branch falls through instead of short-circuiting. In complete_word(), an empty result is then treated as “no known completion” and we fall back to complete.run/filesystem suggestions, so pressing Tab for examples/env-choices.usage.kdl with DEPLOY_ENVS unset will suggest arbitrary paths even though the parser rejects every value. Treating the presence of arg.choices as authoritative here avoids advertising impossible completions.
Useful? React with 👍 / 👎.
|
Addressed the latest completion feedback in Changes:
Verification run:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de8e0d15a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if validate_choices( | ||
| spec, | ||
| &out.cmd, | ||
| &mut out.errors, | ||
| "arg", | ||
| &arg.name, | ||
| &w, | ||
| arg.choices.as_ref(), | ||
| custom_env, |
There was a problem hiding this comment.
The new choice check only runs while consuming argv words here. Parser::parse still fills missing args/flags from arg.env, flag.env, and defaults afterwards without rechecking them, so a spec like arg <env> env="CURRENT_ENV" { choices env="DEPLOY_ENVS" } will accept CURRENT_ENV=prod even when DEPLOY_ENVS=dev,staging. That means the new choices env=... constraint is bypassed whenever the value comes from env/defaults instead of the command line.
Useful? React with 👍 / 👎.
cli/src/cli/complete_word.rs
Outdated
| if let Some(default_cmd) = spec.cmd.find_subcommand(default_name) { | ||
| // Include completions from default subcommand's first arg | ||
| if let Some(arg) = default_cmd.args.first() { | ||
| has_explicit_choices = has_explicit_choices || arg.choices.is_some(); |
There was a problem hiding this comment.
This marks has_explicit_choices based on the default subcommand's first arg even when the active completion target is still a root positional arg. In CompleteWord::complete_word, that suppresses the file fallback at lines 161-165, so a command shaped like cmd <path> loses filesystem suggestions as soon as its default_subcommand happens to start with a choices-constrained arg.
Useful? React with 👍 / 👎.
|
Addressed the latest two review comments in Fixes:
Regression tests added:
Targeted verification:
|
There was a problem hiding this comment.
💡 Codex Review
usage/cli/src/cli/complete_word.rs
Lines 146 to 147 in 82adee1
Mark default-subcommand choices before falling back to files
When the root command has no positional arg of its own but default_subcommand does, this branch appends the default subcommand’s first-arg completions without updating has_explicit_choices. If the current token matches none of those choices, line 161 falls back to filesystem suggestions even though parse() will route that word into the default subcommand and reject every path. A spec like default_subcommand "run" with cmd "run" { arg "<env>" { choices "prod" "staging" } } will therefore suggest files for tool x<Tab> instead of returning no completions.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !arg.default.is_empty() { | ||
| for default_value in &arg.default { | ||
| validate_choices( | ||
| self.spec, | ||
| &out.cmd, |
There was a problem hiding this comment.
For non-variadic args, parse() still returns only arg.default[0], but this new loop rejects every entry in arg.default before we reach that branch. Specs built with SpecArgBuilder::default_values([..]) (which the builder tests already allow for scalar args) will now start failing choice validation on later defaults that are never observable at runtime—for example, default_values(["dev", "prod"]) plus choices ["dev"] now errors even though parsing would still yield "dev". The same regression is repeated in the nested flag-arg default block below.
Useful? React with 👍 / 👎.
1 participant
Summary
envsupport tochoicesso specs can source allowed values from an environment variableParser::with_env(...)without falling back to the process environment when a custom env map is providedchoices env=...resolves to no values instead of silently disabling validationchoices env=...whenSpecArgBuilder::choices()is called afterchoices_env(...)choices env=...in help/docs instead of snapshotting live env-backed values into generated outputusage generate figdoes not bake generation-time environment values into a static fileExample
With
DEPLOY_ENVS="foo,bar baz", valid values becomefoo,bar, andbaz.Implementation notes
choicesnow accepts an optionalenvproperty and preserves it when serializing back to KDLParser::with_env(...)is used; it only consults the process environment when no custom env map is providedSpecArgBuilder::choices()now preserves any previously configuredchoices_env(...)Tests
Parser::with_env(...).choices_env(...).choices(...)ordering